Skip to content

fix: use abi.encode for sender in CCIPLocalSimulatorFork v1.6 messages - #63

Open
kurogami-ibrahim79 wants to merge 1 commit into
smartcontractkit:developfrom
kurogami-ibrahim79:fix/ccip-v1.6-sender-encoding
Open

fix: use abi.encode for sender in CCIPLocalSimulatorFork v1.6 messages#63
kurogami-ibrahim79 wants to merge 1 commit into
smartcontractkit:developfrom
kurogami-ibrahim79:fix/ccip-v1.6-sender-encoding

Conversation

@kurogami-ibrahim79

@kurogami-ibrahim79 kurogami-ibrahim79 commented Sep 2, 2026

Copy link
Copy Markdown

Fixes #62.

Description

This PR addresses the byte-length mismatch for the sender field in CCIP v1.6+ simulated messages.

Previously, CCIPLocalSimulatorFork._executePostV1dot6 used abi.encodePacked(message.sender), which produces a 20-byte payload. On production EVM source chains for v1.6 lanes, this field is expected to be a 32-byte ABI word, matching the fix previously applied in #52 for receiver, destTokenAddress, and sourcePoolAddress.

This mismatch caused receiver contracts to revert when executing abi.decode(message.sender, (address)) or to fail trustedRemote validations due to byte misalignment.

Changes

  • src/ccip/CCIPLocalSimulatorFork.sol: changed abi.encodePacked(message.sender) to abi.encode(message.sender) in _executePostV1dot6, enforcing a 32-byte ABI word with left-zero padding.
  • test/unit/ccip/CCIPLocalSimulatorForkRouting.t.sol: added regression test test_executePostV1dot6_senderIs32ByteABIWord which drives _executePostV1dot6 with a mock OffRamp (MockOffRampCapture) and asserts the captured sender is a 32-byte ABI word.
  • CHANGELOG.md: added an entry under ### Fixed in the Unreleased section.

Testing

  • test_executePostV1dot6_senderIs32ByteABIWord passes. Before the fix, this test fails because abi.encodePacked produces a 20-byte payload.
  • CCIPLocalSimulatorUnitTest and CCIPLocalSimulatorForkRoutingTest pass locally (forge test --match-path "test/unit/ccip/**/*.sol").
  • Fork-based E2E tests are not part of this PR. As noted by @Yurii3721, Sepolia → Arb Sepolia is already on CCIP 2.0, which the simulator does not yet support. The regression test intentionally avoids fork dependencies.

Notes

  • Target branch: develop (per the release playbook).
  • No version bump included; left for maintainers.

@Yurii3721

Yurii3721 commented Sep 3, 2026

Copy link
Copy Markdown

Thank you for looking into this. I am not the maintainer, but, based on the repo’s conventions, there are a few things before this commit can be merged.

The release playbook states that fixes are pushed to develop, whilst main receives updates from it.

It makes sense to add a regression test: verify that sender in a v1.6 message are a 32-byte word decoded into address. CCIPLocalSimulatorForkRouting.t.sol shows harness/mock templates.
Also, worth adding lastSender to MinimalStringReceiver and the check in test_routerDirectSend_routesWhenMultipleOffRampsShareSourceSelector. However, Sepolia->Arb Sepolia seems to be on CCIP 2.0 already. Maybe pin the block until for now? The simulator does not support OnRamp 2, I suppose it will be updated later by the maintainers.
Besides, the tests mentioned in the PR description are unaffected by this change as they do not go through _executePostV1dot6.

And add an entry to the ### Fixed section of the changelog (given the format for 0.2.9). The version bump is likely handled by the maintainers.

@andrejrakic

Copy link
Copy Markdown
Collaborator

Hello @Yurii3721 , thanks for raising the issue, using Chainlink Local and apologies for inconvenience. @Nalon is owning this fix and we will cut the new release as soon as possible.

@kurogami-ibrahim79 thank you for opening this PR and willingness to contribute to Chainlink Local. @Yurii3721 is right, we can not merge this to the main branch directly, there is a process of merging to develop, cutting a beta release, testing and then finally merging to main and cutting a stable release. More details here and here.

@Nalon can you please assist with this PR and make sure this is very well tested? Thanks in advance!

@kurogami-ibrahim79
kurogami-ibrahim79 changed the base branch from main to develop September 8, 2026 15:19
@kurogami-ibrahim79

Copy link
Copy Markdown
Author

@andrejraki Thanks for the explanation regarding the release process. I’ve changed the PR target to the develop branch and will follow the guidelines for the release from beta to stable. Thanks for the guidance. Sorry for the delayed reply.

@kurogami-ibrahim79

Copy link
Copy Markdown
Author

Thanks for the review, @Yuri3721. I will add a regression test to CCIPLocalSimulatorForkRouting.t.sol to ensure the sender is correctly decoded from a 32-byte word into an address, and then add an entry to the ### Fixed section of CHANGELOG.md for version 0.2.9.

@kurogami-ibrahim79
kurogami-ibrahim79 force-pushed the fix/ccip-v1.6-sender-encoding branch 3 times, most recently from 722571c to f3107fe Compare September 10, 2026 06:41
@kurogami-ibrahim79
kurogami-ibrahim79 force-pushed the fix/ccip-v1.6-sender-encoding branch from f3107fe to 63cb773 Compare September 10, 2026 07:15
@kurogami-ibrahim79

Copy link
Copy Markdown
Author

Done. Thanks for the detailed review, @Yurii3721.

Regression test: added test_executePostV1dot6_senderIs32ByteABIWord in CCIPLocalSimulatorForkRouting.t.sol. It drives _executePostV1dot6 with a mock OffRamp (MockOffRampCapture) and asserts the captured sender is a 32-byte ABI word. Before the fix, the test fails; after the fix, it passes.

I also added MockOffRampCapture (recording mock) alongside the existing OffRamp mocks, following the harness/mock pattern in that file. Note: I kept the assertion at the mock boundary (checking message.sender.length == 32) instead of going through MinimalStringReceiver, because the mock OffRamp intercepts before the router dispatch. This avoids the fork dependency entirely and runs cleanly in CI without RPC env vars.

CHANGELOG: added an entry under ### Fixed in the Unreleased section (no version bump, left to maintainers).

Target branch: retargeted to develop.

On the fork test / Sepolia → Arb Sepolia CCIP 2.0 note: you were right, the E2E fork test hit UnsupportedDestinationChain for the Arb Sepolia selector. I've removed that fork test entirely in favor of the mock-based regression test above, which is stable across chain upgrades.

All tests pass locally:

  • CCIPLocalSimulatorUnitTest: 8/8
  • CCIPLocalSimulatorForkRoutingTest: 8/8 (including the new regression test)

Ready for re-review when you have time. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: CCIPLocalSimulatorFork builds Any2EVMRampMessage.sender with abi.encodePacked (20 bytes) for v1.6 messages

3 participants